Port "wapm install" to Wasmer #3317

Michael-F-Bryan · 2022-11-17T11:06:56Z

This ports the wapm install command (wasmerio/wapm-cli#262) to the wasmer CLI under the name wasmer add.

fschutt

Okay overall, just make sure we don't implement the SplitVersion x number of times, since we seem to reuse it for specifying generators, bindings, install, run, etc.

fschutt · 2022-11-17T12:51:00Z

lib/cli/src/commands/install.rs

+    }
+
+    fn target(&self) -> Target {
+        match (self.pip, self.npm, self.yarn) {


I would refactor this to:

if self.pip { return Target::Pip; } if self.npm { return Target::Npm } if self.yarn { return Target::Yarn } unreachable!("Clap should ensure at least one item in the \"bindings\" group is specified")

I'd prefer to keep the match version because it shows that exactly one of pip, npm, and yarn can be set.

I don't want a potential update to those fields (e.g. updating the #[clap] attribute to clap 4.0 or adding a new package manager) to mess up the logic so --npm --pip is accidentally allowed and we silently favour pip.

fschutt · 2022-11-17T12:55:21Z

lib/cli/src/commands/install.rs

+
+/// The full name and optional version number for a WAPM package.
+#[derive(Debug)]
+struct PackageSpecifier {


Technically you can use SplitVersion from crate::cli, which also has tests. I would want to avoid making n number of copies for parsing the namespace/name@version thing.

wasmer/lib/cli/src/cli.rs

Line 274 in 0d5e7d9

SplitVersion::new("registry.wapm.io/graphql/python/python").unwrap(),

I've switched the Add command over to SplitVersion.

At some point we'll need to refactor SplitVersion. Currently it's fulfilling multiple somewhat contradictory roles (URL dependency, a package name, a package's command, etc.) and we're silently ignoring issues (e.g. wasmer add --yarn namespace/package:command would be permitted, but doesn't make logical sense).

fschutt · 2022-11-17T12:56:23Z

lib/registry/src/lib.rs

@@ -347,6 +350,15 @@ pub enum Registries {
    Multi(MultiRegistry),
 }

+impl Registries {
+    pub fn current(&self) -> &str {


This will likely conflict with my code, just copy the entire impl Registries block from wapm-cli.

It's not wrong, but I will have to overwrite it when I port the other commands from wapm-cli.

I've copied the impl Registries block across. You'll probably get some small merge conflicts because I replaced println!() statements with log::warn!() and friends - libraries shouldn't be printing to stdout.

fschutt · 2022-11-17T12:58:26Z

lib/registry/src/lib.rs

@@ -1296,6 +1308,30 @@ pub struct BindingsGenerator {
    pub command: String,
 }

+impl Display for BindingsGenerator {


Isn't this the same syntax as crate::cli::SplitVersion again? Want to avoid having n number of parsers / displayers for the same syntax

Good catch! I didn't know that had been ported across from wapm, so I implemented it again.

Actually, it's not quite the same as SplitVersion because this includes a command in the output and the struct has different fields. The SplitVersion type is also part of the wasmer-cli crate, so I can't import it.

fschutt · 2022-11-17T13:00:46Z

CHANGELOG.md

@@ -10,6 +10,8 @@ Looking for changes that affect our C API? See the [C API Changelog](lib/c-api/C

 ## Added

+- [#3317](https://github.com/wasmerio/wasmer/pull/3317) Add a `wasmer add` command for adding bindings for a WAPM package to your project (only Python and JavaScript are supported for now)


By the way, you don't need to update the changelog at all in PRs. When doing a release, I just do gh search --prs --merged --repo wasmerio/wasmer and then copy the list since the last release. Otherwise I'd have to look if every PR has included this message. So this commit is nice, but pointless.

Ah okay. We should probably update the pull request template so we don't tell people to update the changelog.

Michael-F-Bryan · 2022-11-17T19:34:40Z

bors r+

bors · 2022-11-17T19:44:55Z

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

{"message":"Changes must be made through a pull request.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

Michael-F-Bryan requested a review from fschutt November 17, 2022 11:06

Michael-F-Bryan marked this pull request as ready for review November 17, 2022 12:23

Michael-F-Bryan requested a review from syrusakbary as a code owner November 17, 2022 12:23

fschutt reviewed Nov 17, 2022

View reviewed changes

Michael-F-Bryan force-pushed the wasmer-install branch from f111faf to 660992c Compare November 17, 2022 16:12

Michael-F-Bryan added 10 commits November 18, 2022 01:59

Update the GraphQL schema to be in sync with wapm.io

af73775

Fixed a typo in Cargo.toml which caused warnings

fe2e2b4

List bindings associated with a package

60f4ffa

Added an install command

e630c50

Add install to the list of supported commands

02e6130

Renamed "wasmer install" to "wasmer add"

8d46d34

Update the changelog

0830f10

Delete PackageSpecifier in favour of SplitVersion

3f09c82

Copy across "impl Registries" from the wapm CLI

ac67997

Fixed a broken doc-link

c3ebf06

Michael-F-Bryan force-pushed the wasmer-install branch from 660992c to c3ebf06 Compare November 17, 2022 17:59

bors bot merged commit 9401599 into master Nov 17, 2022

bors bot deleted the wasmer-install branch November 17, 2022 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port "wapm install" to Wasmer #3317

Port "wapm install" to Wasmer #3317

Michael-F-Bryan commented Nov 17, 2022 •

edited

Loading

fschutt left a comment

fschutt Nov 17, 2022

Michael-F-Bryan Nov 17, 2022

fschutt Nov 17, 2022

Michael-F-Bryan Nov 17, 2022 •

edited

Loading

fschutt Nov 17, 2022

fschutt Nov 17, 2022

Michael-F-Bryan Nov 17, 2022

fschutt Nov 17, 2022

Michael-F-Bryan Nov 17, 2022

Michael-F-Bryan Nov 17, 2022

fschutt Nov 17, 2022

Michael-F-Bryan Nov 17, 2022

Michael-F-Bryan commented Nov 17, 2022

bors bot commented Nov 17, 2022

		@@ -10,6 +10,8 @@ Looking for changes that affect our C API? See the [C API Changelog](lib/c-api/C

		## Added

		- [#3317](https://github.com/wasmerio/wasmer/pull/3317) Add a `wasmer add` command for adding bindings for a WAPM package to your project (only Python and JavaScript are supported for now)

Port "wapm install" to Wasmer #3317

Port "wapm install" to Wasmer #3317

Conversation

Michael-F-Bryan commented Nov 17, 2022 • edited Loading

fschutt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-F-Bryan Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-F-Bryan commented Nov 17, 2022

bors bot commented Nov 17, 2022

Michael-F-Bryan commented Nov 17, 2022 •

edited

Loading

Michael-F-Bryan Nov 17, 2022 •

edited

Loading